-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Chan Jing Rong] Duke Increments #346
base: master
Are you sure you want to change the base?
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)
Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.
Add configuration for console applications
I like your commit messages. They all begin with a base verb and are very informative and consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your import lines should not have new lines between them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the overall structure of your code. Keep up the good work :)
src/main/java/Ui.java
Outdated
public void execute(TaskList tasks) { | ||
String command = scanner.nextLine(); | ||
showLine(); | ||
if (command.equals("bye")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using switches if the if else statements get too messy
|
||
public class Deadline extends Task { | ||
private String name; | ||
private boolean done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should change done to isDone() to follow variable naming.
|
||
public Parser() { | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can choose not to type out the constructor method
task = new Deadline(words[2], words[3]); | ||
} else { | ||
task = new Event(words[2], words[3]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using switch if your if-else statements are long
src/main/java/Task.java
Outdated
@@ -0,0 +1,37 @@ | |||
public abstract class Task { | |||
private String name; | |||
private boolean done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done can be renamed isDone
src/main/java/Task.java
Outdated
@@ -0,0 +1,37 @@ | |||
public abstract class Task { | |||
private String name; | |||
private boolean done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name and done should be protected instead, so that classes that extends it can use it
@@ -0,0 +1,17 @@ | |||
public class Todo extends Task { | |||
private String name; | |||
private boolean done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not recreate a new private variable in Todo. Instead use Task's protected isDone and name
|
||
@Override | ||
public String toString() { | ||
return String.format("[T]%s", super.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of String.format() here
src/main/java/Ui.java
Outdated
int index = Integer.valueOf(commands[1]) - 1; | ||
System.out.println(" " + tasks.getTasks().get(index).toString()); | ||
tasks.delete(index); | ||
System.out.println(String.format(" Now you have %d tasks in the list.", tasks.getTasks().size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are reusing tasks.getTasks() multiple times, consider creating a variable just to store the reference
src/main/java/Ui.java
Outdated
System.out.println(); | ||
} | ||
|
||
public void execute(TaskList tasks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the naming of the parameter : tasks, as it suggest it is a list, not one item
* @return Formatted description of task. | ||
*/ | ||
@Override | ||
public String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work in abstracting common parts of code and using it in subclasses with super()
} | ||
|
||
Task task; | ||
if (commands[0].equals("todo") || commands[0].equals("deadline") || commands[0].equals("event")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since lines are getting long and commands[0] is used multiple times, maybe assign String move = commands[0] and use move for proceeding lines?
No description provided.